Description

LocalhostOnlyMiddleware only enforces loopback when request.client is present and explicitly allows
non-IP client hosts, so /internal/* routes can be reached without a verified localhost source in
some ASGI/proxy/test setups. This weakens the intended protection for the internal git-tools
endpoint.

Code

webhook_server/web/git_tools.py[R30-38]

+    async def dispatch(self, request: Request, call_next: RequestResponseEndpoint) -> Response:
+        if request.url.path.startswith("/internal/") and request.client:
+            client_host = request.client.host
+            try:
+                ip = ipaddress.ip_address(client_host)
+                if not ip.is_loopback:
+                    return JSONResponse(status_code=403, content={"detail": "Internal endpoints are localhost-only"})
+            except ValueError:
+                pass  # Non-IP host (e.g., test client) — allow

Relevance

⭐⭐⭐ High

Team often tightens security gates (e.g., add trusted-network deps, fail-safe bot-ownership checks)
— likely accept fail-closed localhost check.

PR-#982
PR-#1108

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The middleware currently gates on and request.client and treats non-IP hosts as allowed, which
means the localhost restriction is not enforced when client info is unavailable or unparsable.
Another security dependency in app.py demonstrates the codebase expects request.client/IP parsing
to fail and denies in that situation, supporting that this should fail closed here too.

webhook_server/web/git_tools.py[1-7]
webhook_server/web/git_tools.py[27-39]
webhook_server/app.py[117-133]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`LocalhostOnlyMiddleware` is intended to make `/internal/*` endpoints localhost-only, but it currently fails open in two cases:
1) when `request.client` is missing, and
2) when `request.client.host` is not parseable as an IP (`ValueError`), where it currently allows the request.

This can allow access to internal endpoints without a verified loopback source.

### Issue Context
The module docstring claims the endpoints are “Bound to 127.0.0.1 only”, and other security checks in the codebase (e.g., `require_trusted_network`) deny access when the client IP is missing/invalid. For consistency and safety, the internal middleware should also fail closed.

### Fix Focus Areas
- webhook_server/web/git_tools.py[30-39]

### Suggested change
- If the request path starts with `/internal/` and `request.client` is missing, return 403.
- If `ipaddress.ip_address(client_host)` raises `ValueError`, return 403 (or explicitly allow only known-safe test values behind a test-only flag).
- Optionally add a small log message when denying due to missing/invalid client IP to aid debugging in unusual deployments.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

call_ai() calls pi_sidecar_client.call_ai_once directly without checking that the pi-sidecar
server is reachable/healthy first. This can cause opaque runtime failures when the sidecar is
unavailable, instead of failing with a clear availability error as required.

Code

webhook_server/libs/ai_cli.py[R10-34]

+async def call_ai(
    prompt: str,
    ai_provider: str,
    ai_model: str,
    cwd: str,
-    cli_flags: list[str] | None = None,
    timeout_minutes: int | None = None,
-) -> tuple[bool, str]:
-    """Call an AI CLI tool. Thin wrapper around ai_cli_runner.call_ai_cli.
+    system_prompt: str = "",
+    tools: list[str] | None = None,
+    custom_tools: list[dict[str, Any]] | None = None,
+) -> AIResult:
+    """Call an AI provider via pi-sidecar. Thin wrapper around pi_sidecar_client.call_ai_once.

-    Accepts cwd as str (matching clone_repo_dir type) and converts to Path.
+    Returns:
+        AIResult with .success, .text, and .error attributes.
    """
-    return await _call_ai_cli(
+    return await call_ai_once(
        prompt=prompt,
        ai_provider=ai_provider,
        ai_model=ai_model,
-        cwd=Path(cwd),
-        cli_flags=cli_flags,
-        ai_cli_timeout=timeout_minutes,
+        cwd=cwd,
+        ai_call_timeout=timeout_minutes,
+        system_prompt=system_prompt,
+        tools=tools,
+        custom_tools=custom_tools,
    )

Relevance

⭐⭐⭐ High

Team previously added preflight health check before external AI service calls (test-oracle) in PR
#1002.

PR-#1002

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 181407 requires an availability/health check prior to invoking AI. The new
call_ai() wrapper directly invokes call_ai_once(...) with no preflight health check or explicit
unavailable-sidecar error handling.

Ensure deployment starts/has access to pi-sidecar server before AI calls
webhook_server/libs/ai_cli.py[10-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`webhook_server/libs/ai_cli.py:call_ai()` calls pi-sidecar directly but does not perform any explicit availability/health check before issuing AI requests.

## Issue Context
Compliance requires the application to verify that the pi-sidecar server is started/reachable before AI calls, and to fail with a clear error when it is not.

## Fix Focus Areas
- webhook_server/libs/ai_cli.py[10-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

In _get_ai_title_suggestion(), the non-wildcard regex captures only the conventional commit type
(e.g., "fix") but not the full title (e.g., "fix: ..."), because only the allowed-types alternation
is wrapped in the capture group. This can cause the check to return an invalid suggestion even when
the AI response contains a valid conventional title.

Code

webhook_server/libs/handlers/runner_handler.py[R992-1001]

+                    if is_wildcard or not allowed_names:
+                        # Wildcard mode: match any word followed by optional scope and colon
+                        title_pattern = r"(?:^|\n|[.!?]\s*)(\w+(?:\([^)]*\))?!?:\s*\S.+)"
+                    else:
+                        allowed_types_pattern = "|".join(re.escape(name) for name in allowed_names)
+                        title_pattern = rf"(?:^|\n|[.!?]\s*)({allowed_types_pattern})(?:\([^)]*\))?!?:\s*\S.+"
+                    match = re.search(title_pattern, response_text, re.IGNORECASE)
+                    if match:
+                        suggestion = match.group(1).strip().strip("`").strip('"').strip("'")
+                    else:

Relevance

⭐⭐⭐ High

Team frequently accepts correctness fixes in runner_handler regex/title logic (e.g.,
conventional-title regex updates merged).

PR-#900
PR-#1080

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The pattern in the non-wildcard path captures only ({allowed_types_pattern}), but the code returns
match.group(1), so the suggestion becomes only the type token instead of the full title line.

webhook_server/libs/handlers/runner_handler.py[988-1004]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_get_ai_title_suggestion()` uses `match.group(1)` as the suggestion. In the non-wildcard branch, the capture group only wraps the allowed type token, so the returned suggestion is just the type (e.g., `fix`) instead of the full conventional title line (e.g., `fix: correct the title`).

### Issue Context
This logic runs when AI title suggestions are enabled and `allowed_names` is non-empty.

### Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[992-1004]

### Suggested fix
Update the non-wildcard `title_pattern` so the *entire* conventional-title string is captured in group(1), e.g.:
- `title_pattern = rf"(?:^|\n|[.!?]\s*)({allowed_types_pattern}(?:\([^)]*\))?!?:\s*\S.+)"`

Keep `suggestion = match.group(1)...` as-is after the regex fix.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

test_git_tools patches asyncio.create_subprocess_shell with a non-awaitable mock, but
run_git_command() awaits it, which will raise a TypeError and break the new test suite. This
makes the added coverage unreliable and can fail CI on this PR.

Code

webhook_server/tests/test_git_tools.py[R21-27]

+    def test_allowed_command_diff(self, client: TestClient) -> None:
+        with patch("webhook_server.web.git_tools.asyncio.create_subprocess_shell") as mock_proc:
+            process = AsyncMock()
+            process.communicate.return_value = (b"file.py | 2 +-\n", b"")
+            process.returncode = 0
+            mock_proc.return_value = process
+

Relevance

⭐⭐⭐ High

Similar async-test mock awaitability fixes were accepted before (patch non-awaitable causing
TypeError) in PR #906.

PR-#906

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The endpoint awaits asyncio.create_subprocess_shell, but the tests patch it without
new_callable=AsyncMock, so the patched symbol will not be awaitable and the await will fail.

webhook_server/web/git_tools.py[32-55]
webhook_server/tests/test_git_tools.py[21-27]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`run_git_command()` does `proc = await asyncio.create_subprocess_shell(...)`, but the tests patch `create_subprocess_shell` using the default (sync) mock, which is not awaitable. This will fail when the endpoint code awaits the patched function.

### Issue Context
The new tests in `webhook_server/tests/test_git_tools.py` repeatedly use:
```py
with patch("...asyncio.create_subprocess_shell") as mock_proc:
   mock_proc.return_value = process
```
This should instead patch the coroutine with an `AsyncMock` so `await asyncio.create_subprocess_shell(...)` returns the mocked process.

### Fix Focus Areas
- webhook_server/tests/test_git_tools.py[21-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The /internal/git-tools router is added to the main FastAPI app without any access control, and
the server defaults to binding on 0.0.0.0, so this “internal” endpoint can be reachable off-host
if the service port is published.

Code

webhook_server/app.py[R301-302]

FASTAPI_APP: FastAPI = FastAPI(title="webhook-server", lifespan=lifespan)
+FASTAPI_APP.include_router(git_tools_router)

Relevance

⭐⭐⭐ High

Team often gates risky FastAPI endpoints via env/deps (ENABLE_LOG/MCP, require_trusted_network).
Unconditional internal router likely changed.

PR-#844
PR-#916
PR-#982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The router is included unconditionally in the app, and the process entrypoint shows the server binds
to 0.0.0.0 by default, so the endpoint is not inherently localhost-only.

webhook_server/app.py[55-56]
webhook_server/app.py[300-303]
entrypoint.py[13-17]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`/internal/git-tools/run` is mounted into the public FastAPI app and currently has no enforcement that requests originate from localhost, despite being documented as “Bound to 127.0.0.1 only”. This can make the endpoint accessible to external callers depending on deployment/networking.

### Issue Context
The server’s default bind address is `0.0.0.0`, which listens on all interfaces.

### Fix Focus Areas
- webhook_server/app.py[300-303]
- webhook_server/web/git_tools.py[19-45]

### Implementation guidance
Choose one (or combine):
- Add a dependency / middleware for the `/internal/*` router that rejects any request whose `request.client.host` is not `127.0.0.1` / `::1`.
- Alternatively require a shared secret header (set in container env) and validate it in the endpoint/dependency.
- Update/adjust tests to set an allowed client host when using `TestClient`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The updated cherry-pick conflict-resolution prompt includes commit context and target branch but
does not include the original PR title (pull_request.title) as required. Missing this context can
reduce resolution quality and increase semantically incorrect conflict fixes.

Code

webhook_server/libs/handlers/runner_handler.py[R1229-1236]

        prompt = (
            "You are in a git repository with cherry-pick merge conflicts. "
            "Resolve ALL conflicts in ALL files.\n\n"
-            "How to handle each conflict type:\n"
+            f"## Original Commit Context\n"
+            f"**Commit:** `{commit_hash}`\n"
+            f"**Message:** {commit_message}\n"
+            f"**Target branch:** `{target_branch}`\n\n"
+            f"**Original commit changed files:**\n```\n{commit_diff_stat}\n```\n\n"

Relevance

⭐⭐⭐ High

Team accepts adding missing AI prompt context; they previously added base-branch context to AI
prompts in PR #1080.

PR-#1080
PR-#1025

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance ID 181410 requires the conflict-resolution prompt to include the original PR title and
target branch. The prompt currently includes commit_hash, commit_message, and target_branch,
but contains no PR title context.

Improve conflict-resolution prompt with original commit and PR/branch context
webhook_server/libs/handlers/runner_handler.py[1229-1236]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cherry-pick conflict-resolution AI prompt is required to include the original PR title (`pull_request.title`), but the current prompt only includes commit hash/message and target branch.

## Issue Context
Compliance ID 181410 requires adding original PR title and target branch context to reduce incorrect AI resolutions.

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[1160-1271]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The cherry-pick conflict-resolution prompt instructs the AI to view the original commit diff, but
does not instruct comparing the resolved state against that original diff. This violates the
requirement to explicitly verify the resolution preserved the original intent.

Code

webhook_server/libs/handlers/runner_handler.py[R1237-1245]

+            "## Instructions\n\n"
+            f"Run `git log --oneline -1 {commit_hash}` to understand the original intent.\n"
+            f"Run `git diff {commit_hash}^..{commit_hash}` to see the original changes.\n\n"
+            "### Conflict Resolution Rules\n"
+            "- **Prefer the cherry-picked changes.** Only use HEAD (target branch) when "
+            "the cherry-picked code references APIs/functions that don't exist on the target branch.\n"
            "- Standard conflict markers (<<<<<<< HEAD, =======, >>>>>>>): "
-            "HEAD is the target branch. Adapt the cherry-picked changes to fit "
-            "the target branch code.\n"
+            "HEAD is the target branch. Resolve in favor of the cherry-picked changes "
+            "unless they reference code not present on the target branch.\n"

Relevance

⭐⭐⭐ High

Team has repeatedly merged stronger AI cherry-pick guidance/verification behavior; prompt
improvements landed in PR #1046.

PR-#1046
PR-#1025

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance ID 181412 requires an explicit prompt instruction to compare the resolved result against
the original commit diff. The current prompt only instructs running `git diff
{commit_hash}^..{commit_hash}` to view original changes, but does not instruct comparing the
post-resolution state to that diff.

Add AI instruction to verify resolution against original diff
webhook_server/libs/handlers/runner_handler.py[1237-1245]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The AI prompt lacks an explicit instruction to compare the final resolved/staged changes against the original commit diff to ensure no intended changes were dropped.

## Issue Context
Compliance ID 181412 requires adding a prompt instruction to verify the resolution against the original diff (e.g., compare resolved changes vs `git diff {commit_hash}^..{commit_hash}`).

## Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[1237-1256]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

After AI resolves conflicts, _resolve_cherry_pick_with_ai() runs git add -u, which does not
stage newly-added/untracked files. This can make cherry-pick --continue fail or produce a
cherry-pick commit that silently omits files introduced by the cherry-picked change.

Code

↗ webhook_server/libs/handlers/runner_handler.py

            )

Relevance

⭐⭐⭐ High

Team previously staged all changes with git add -A in cherry-pick automation; similar staging
robustness accepted in PR #1108.

PR-#1108

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The AI resolution finalization stages with git add -u, while the same cherry-pick flow later uses
git add -A to ensure all modifications (including new files) are staged for pre-commit auto-fix
commits—highlighting that -A is the expected behavior when you must not miss new files.

webhook_server/libs/handlers/runner_handler.py[1280-1310]
webhook_server/libs/handlers/runner_handler.py[1597-1604]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
After AI conflict resolution, the code stages changes with `git add -u`, which does not include newly-added files. This can break cherry-pick continuation and/or drop new files from the resulting commit.

### Issue Context
This occurs in `RunnerHandler._resolve_cherry_pick_with_ai()` right before the CHERRY_PICK_HEAD check and `cherry-pick --continue`.

### Fix Focus Areas
- webhook_server/libs/handlers/runner_handler.py[1280-1289]

### Proposed fix
- Replace `git add -u` with `git add -A` (or `git add --all`) so new files created/kept during conflict resolution are staged.
- Keep the existing error handling, but ensure the staging step truly stages *everything* the AI resolved.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The new sidecar build stage runs npm ci, but sidecar-helper/package-lock.json pins transitive
dependencies to git+ssh://git@github.com/..., which requires SSH credentials during the Docker
build and can fail in CI/build environments that don’t provide SSH keys/agent forwarding.

Code

Dockerfile[R1-8]

+# Sidecar build stage
+FROM node:22-slim AS sidecar-builder
+WORKDIR /sidecar
+COPY sidecar-helper/package.json sidecar-helper/package-lock.json* ./
+RUN npm ci
+COPY sidecar-helper/ .
+RUN npx tsc
+RUN npm prune --omit=dev

Relevance

⭐⭐ Medium

No direct history on git+ssh npm deps; team does act on Docker build reliability issues (e.g., PR
#965).

PR-#965

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR introduces a new Docker build stage that executes npm ci, and the checked-in lockfile shows
pi-orchestrator-config and pi-vertex-claude resolved via git+ssh://git@github.com/..., which
requires SSH access during install.

Dockerfile[1-8]
sidecar-helper/package-lock.json[2716-2737]
sidecar-helper/package-lock.json[1466-1479]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Docker sidecar build runs `npm ci`, but the lockfile contains `git+ssh://git@github.com/...` resolved URLs for transitive dependencies. This typically breaks container builds (and reproducibility) unless the build environment is explicitly configured with SSH credentials and git/ssh tooling.

## Issue Context
`npm ci` strictly follows the lockfile; `git+ssh` URLs require an SSH key and an SSH client.

## Fix Focus Areas
- Dockerfile[1-8]
- sidecar-helper/package-lock.json[2716-2737]

## Recommended fix
- Regenerate `sidecar-helper/package-lock.json` in a clean environment so GitHub dependencies resolve via `git+https://...` (or npm registry tarballs) rather than `git+ssh://...`.
- If SSH-based deps are truly required (e.g., private repos), update the sidecar-builder stage to install `git` + `openssh-client` and use an explicit, secure SSH provisioning mechanism during build (e.g., BuildKit SSH mount), documenting it in build instructions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

run_git_command() forwards unfiltered git diff arguments, allowing `git diff --no-index
/path/to/secret /dev/null` to return arbitrary filesystem contents if the endpoint is reachable or
the AI is prompt-injected into invoking it. This breaks the intended “read-only git tooling”
boundary and can leak secrets from the container/mounted volumes.

Code

webhook_server/web/git_tools.py[R35-55]

+    parts = shlex.split(request.args)
+    if not parts:
+        raise HTTPException(status_code=400, detail="Empty git command")
+
+    subcommand = parts[0]
+    if subcommand not in ALLOWED_GIT_COMMANDS:
+        raise HTTPException(
+            status_code=403,
+            detail=f"Git subcommand '{subcommand}' not allowed. Allowed: {', '.join(sorted(ALLOWED_GIT_COMMANDS))}",
+        )
+
+    # Build argument list — no shell interpolation
+    cmd_args = ["git", "-C", request.cwd, *parts]
+    proc: asyncio.subprocess.Process | None = None
+    try:
+        proc = await asyncio.create_subprocess_exec(
+            *cmd_args,
+            stdout=asyncio.subprocess.PIPE,
+            stderr=asyncio.subprocess.PIPE,
+        )
+        stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=30)

Relevance

⭐⭐ Medium

No historical evidence on filtering git diff flags; team does accept internal-endpoint security
hardening (PR #982, #1109).

PR-#982
PR-#1109

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The endpoint parses request.args and passes it directly to git without filtering dangerous
diff flags, and the runner builds AI custom tools that let the AI provide that args string.

webhook_server/web/git_tools.py[32-60]
webhook_server/libs/handlers/runner_handler.py[65-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The internal `/internal/git-tools/run` endpoint allowlists only the git *subcommand* (`diff`, `log`, etc.) but does not restrict subcommand arguments. For `git diff`, the `--no-index` flag enables diffing arbitrary filesystem paths (including absolute paths), which can leak secrets.

## Issue Context
- The endpoint is used by pi-sidecar HTTP-backed custom tools.
- The AI can supply the `args` string (via the custom tool schema), so prompt injection or accidental misuse can trigger dangerous git invocations.

## Fix Focus Areas
- webhook_server/web/git_tools.py[33-60]

### Recommended fix
1. Add per-subcommand argument validation, starting with `diff`:
  - Reject `--no-index` explicitly.
  - Consider rejecting absolute-path operands and `..` path traversal in path arguments after `--`.
2. Optionally narrow the allowed `diff` flags to a safe subset for your use cases (`--stat`, `--name-only`, specific rev ranges, and relative pathspecs).
3. Add a unit test proving `diff --no-index /etc/passwd /dev/null` (or any absolute path) is rejected with 403/400.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

run_git_command() catches Exception and converts it to a success=false response, which can
silently hide unexpected programming/integration failures. This violates the requirement to catch
specific exception types (or log with full context and re-raise at true process boundaries).

Code

webhook_server/web/git_tools.py[R58-59]

+    except Exception as ex:
+        return GitCommandResponse(success=False, output=str(ex))

Relevance

⭐⭐ Medium

Broad Exception sometimes accepted for handler robustness, but usually with
logger.exception/re-raise; here it swallows errors.

PR-#1109
PR-#968
PR-#982

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 181390 disallows broad except Exception in non-boundary code paths unless it is
tightly justified and properly observed. The new endpoint handler catches Exception and returns a
normal response, suppressing unexpected failures.

Rule 181390: Catch specific exception types instead of broad Exception
webhook_server/web/git_tools.py[56-59]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`webhook_server/web/git_tools.py` uses a broad `except Exception as ex:` and returns `str(ex)`, which can suppress unexpected errors.

## Issue Context
This endpoint is invoked during AI sessions; unexpected exceptions should not be silently flattened into a generic response.

## Fix Focus Areas
- webhook_server/web/git_tools.py[56-59]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

run_git_command() validates only the first token of args but then interpolates the full
untrusted request.args into a shell string and executes it with create_subprocess_shell,
allowing ;/&& command injection after an allowed subcommand.

Code

webhook_server/web/git_tools.py[R35-55]

+    parts = shlex.split(request.args)
+    if not parts:
+        raise HTTPException(status_code=400, detail="Empty git command")
+
+    subcommand = parts[0]
+    if subcommand not in ALLOWED_GIT_COMMANDS:
+        raise HTTPException(
+            status_code=403,
+            detail=f"Git subcommand '{subcommand}' not allowed. Allowed: {', '.join(sorted(ALLOWED_GIT_COMMANDS))}",
+        )
+
+    cmd = f"git -C {shlex.quote(request.cwd)} {request.args}"
+    try:
+        proc = await asyncio.create_subprocess_shell(
+            cmd,
+            stdout=asyncio.subprocess.PIPE,
+            stderr=asyncio.subprocess.PIPE,
+        )
+        stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=30)
+        output = stdout.decode() if proc.returncode == 0 else stderr.decode()
+        return GitCommandResponse(success=proc.returncode == 0, output=output[:50000])

Relevance

⭐⭐ Medium

No direct historical reviews on subprocess_shell injection; only general subprocess
hardening/cleanup precedent found.

PR-#919

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The endpoint uses shlex.split() only for the allowlist check, but still executes the raw
request.args through a shell, enabling injected shell metacharacters to be interpreted.

webhook_server/web/git_tools.py[35-55]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`/internal/git-tools/run` is intended to run read-only git commands, but it is currently vulnerable to shell command injection because it executes a string via `asyncio.create_subprocess_shell()` and appends `request.args` directly.

### Issue Context
The handler already parses `request.args` with `shlex.split()` to check the first token, but it then ignores the parsed list and executes the raw string.

### Fix Focus Areas
- webhook_server/web/git_tools.py[35-55]

### Implementation guidance
- Replace `create_subprocess_shell(cmd, ...)` with `create_subprocess_exec(*argv, ...)`.
- Build `argv` from the parsed `parts` (e.g., `argv = ["git", "-C", request.cwd, *parts]`).
- Keep the existing subcommand allowlist check, but apply it to `parts[0]` and only execute the `parts` list.
- Catch `asyncio.TimeoutError` from `wait_for` (or both `asyncio.TimeoutError` and `TimeoutError`) to be robust.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

entrypoint.sh polls the sidecar /health endpoint but still proceeds to start the main app even
if the sidecar never becomes ready, and it only logs a warning when the sidecar script is missing.
This violates the requirement to ensure pi-sidecar availability with a clear failure mode and can
leave the container running while degraded/unhealthy until the orchestrator intervenes.

Code

entrypoint.sh[R5-29]

+if [ -f "$APP_DIR/sidecar-helper/dist/server.js" ]; then
+    export SIDECAR_PORT="${SIDECAR_PORT:-9100}"
+    export SIDECAR_ACPX_EXTENSION_PATH="$APP_DIR/sidecar-helper/node_modules/@myk-org/pi-sidecar/node_modules/pi-orchestrator-config/extensions/acpx-provider/index.ts"
+    node "$APP_DIR/sidecar-helper/dist/server.js" &
+    SIDECAR_PID=$!
+    echo "[sidecar] Started Pi SDK sidecar (PID $SIDECAR_PID) on port $SIDECAR_PORT"
+
+    # Kill sidecar when main process exits
+    trap 'kill $SIDECAR_PID 2>/dev/null; wait $SIDECAR_PID 2>/dev/null' EXIT
+
+    # Monitor sidecar — if it dies, kill the main process too
+    # TERM trap prevents misleading "died" message on normal shutdown
+    (trap 'exit 0' TERM; while kill -0 $SIDECAR_PID 2>/dev/null; do sleep 5; done; echo "[sidecar] Sidecar died, shutting down container"; kill 1 2>/dev/null) &
+
+    # Wait for sidecar to be ready (up to 15s)
+    for i in $(seq 1 30); do
+        if curl -sf http://127.0.0.1:$SIDECAR_PORT/health > /dev/null 2>&1; then
+            echo "[sidecar] Health check passed"
+            break
+        fi
+        sleep 0.5
+    done
+else
+    echo "[sidecar] WARNING: sidecar-helper/dist/server.js not found, AI features will not be available"
+fi

Relevance

⭐⭐ Medium

No historical review evidence for sidecar readiness fail-fast behavior; entrypoint.sh is new (no
file history).

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 181410 requires runtime startup to ensure sidecar availability and to fail
explicitly when the sidecar is missing or unhealthy, but the script only warns if
sidecar-helper/dist/server.js is absent and its readiness loop breaks only on success without any
post-loop handling before exec uv run entrypoint.py if /health never succeeds. Additionally, the
Docker HEALTHCHECK marks the container unhealthy when the sidecar health endpoint is down, so
starting the main server without enforcing sidecar readiness can result in a container that is
"running" yet unhealthy.

Ensure pi-sidecar runtime availability and handle missing sidecar as a failure mode
entrypoint.sh[5-29]
entrypoint.sh[1-32]
Dockerfile[93-107]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`entrypoint.sh` does not enforce that pi-sidecar is present and healthy before starting the main process: it only logs a warning when `sidecar-helper/dist/server.js` is missing, and its `/health` readiness polling loop has no failure handling, so after the wait window it still starts the main app even if readiness was never achieved.

## Issue Context
Compliance (PR Compliance ID 181410) requires (1) runtime startup ensures sidecar availability and (2) missing/unhealthy sidecar is handled explicitly with a clear failure mode. Docker `HEALTHCHECK` also requires both the webhook server and the sidecar to be healthy, meaning the container can appear "running" while remaining unhealthy if the main process starts without a ready sidecar.

## Fix Focus Areas
- entrypoint.sh[5-32]
- Dockerfile[105-107]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools